refactor(tui): extract attachments from document parts in session by Piyush0049 · Pull Request #3484 · docker/docker-agent

Piyush0049 · 2026-07-06T09:15:48Z

Description

This PR resolves a technical debt TODO in pkg/tui/page/chat/chat.go regarding how attachments are retrieved from session history when editing a past message in the TUI.

Previously, extractAttachmentsFromSession relied exclusively on a text-matching fallback ("Contents of <filename>: <dataURL>"). As the codebase has evolved to use MessagePartTypeDocument for structured file attachments, this old extraction logic would fail to properly restore modern attachments to the message editor.

Changes:

Added proper parsing for chat.MessagePartTypeDocument to safely extract InlineText and InlineData.
Implemented automatic conversion of binary InlineData payloads into standard data: URIs (base64) so that image attachments render properly in the UI.
Retained the original "Contents of " string matching strictly as a fallback (legacyPrefix) to ensure backward compatibility with older saved sessions.
Removed the outdated TODO comment.

Testing

go build ./pkg/tui/... succeeds.
go test ./pkg/tui/page/chat/... passes.

This commit updates extractAttachmentsFromSession to properly read modern MessagePartTypeDocument attachments instead of relying solely on the legacy 'Contents of' string matching fallback, addressing the TODO in chat.go.

Piyush0049 · 2026-07-06T10:13:18Z

I just force-pushed to fix a trailing whitespace error that was failing the linter. The CI should pass now.

dgageot · 2026-07-06T12:16:07Z

Blocking issue: binary attachments round-trip as raw base64 text in the prompt.

On the resend path, the extracted attachment has FilePath == "" and Content != "", so App.Run → processInlineAttachment (pkg/app/app.go:638-642) embeds the data:<mime>;base64,... URI as literal prompt text inside <attached_file> tags. Nothing downstream parses it back into an image/document part (parseDataURI is only reached via MessagePartTypeImageURL parts). A 3 MB image becomes ~4 MB of base64 text (~1M tokens) — context overflow or a very expensive garbage request; arguably worse than the previous silent drop. It also violates Attachment.Content's documented "raw text content" contract (pkg/tui/messages/session.go:18-21).

Suggested fixes:

Extend messages.Attachment with MimeType/Data []byte (its doc comment anticipates this) and rebuild a Document part in App.Run, e.g. via chat.ProcessAttachmentWithMetadata; or
As a safe stopgap, extract only InlineText documents and keep skipping InlineData, with a short comment explaining why.

dgageot

See comment above

Piyush0049 · 2026-07-06T12:36:53Z

Thanks for the review. I am working on the suggested fixes.

This fixes a context overflow issue where binary attachments (like images) were being serialized as massive base64 text strings when a message was edited and resent. We now extend messages.Attachment to properly hold MimeType and Data, and rebuild the Document part natively in App.Run.

Piyush0049 · 2026-07-06T13:27:49Z

@dgageot I've implemented the primary suggestion to address the context overflow on the resend path.

Specifically, I:

Extended messages.Attachment with MimeType and Data []byte to natively support binary payloads (as the doc comment anticipated).
Updated the TUI extraction logic in pkg/tui/page/chat/chat.go to stop base64-encoding InlineData.
Added a new branch in App.Run to intercept binary attachments (len(att.Data) > 0) and route them through chat.ProcessAttachmentWithMetadata, safely rebuilding the Document part natively.

Please let me know if this looks good to you.

Sayt-0

Review summary

Corrects a real data loss: editing a past message dropped modern MessagePartTypeDocument attachments because extractAttachmentsFromSession only understood the legacy Contents of <file>: <dataURL> text format. The context-overflow issue from the earlier revision is resolved by extending messages.Attachment and rebuilding a Document part through chat.ProcessAttachmentWithMetadata.

Verified on the PR branch: go build ./pkg/..., go test ./pkg/tui/... ./pkg/app/... ./pkg/chat/..., and golangci-lint all pass.

Area	Status
Correctness (round-trip: text, binary, legacy, multi-attachment)	ok
Image resend idempotence (`Resized:false`, no duplicate note)	ok
Security (size and decompression-bomb guards retained)	ok
Tests	none added
Doc comment on `Attachment`	stale
Unrelated cosmetic edit in `processFileAttachment`	present

Recommendation: request changes (light). The substance is approvable. Before merge: add a regression test for extractAttachmentsFromSession, refresh the stale Attachment doc comment, and drop the unrelated WriteString split. Filed as a comment rather than a blocking status since a changes-requested review is already open.

Sayt-0 · 2026-07-08T13:36:55Z

+	// MimeType is the MIME type of the binary data, if Data is present.
+	MimeType string
+	// Data holds raw binary content for non-text inline attachments.
+	Data []byte


The struct's doc comment at the top of this file is now stale. It still states that the value is either FilePath or Content, and that binary support is a future extension via a MimeType hint. With Data/MimeType added here, it should document the third case (inline binary) and drop the future-tense wording.

Sayt-0 · 2026-07-08T13:36:55Z

 	for i := 1; i < len(msg.MultiContent); i++ {
 		part := msg.MultiContent[i]
+
+		if part.Type == chat.MessagePartTypeDocument && part.Document != nil {


No regression test accompanies this change, and this is the exact path that previously shipped the context-overflow bug. A unit test on extractAttachmentsFromSession covering the four cases (Document with InlineText, Document with InlineData, legacy Contents of text, and an empty Document) would lock the behavior. The existing newTestChatPage harness plus an app.New(...) session can drive it.

Sayt-0 · 2026-07-08T13:36:55Z

 				case att.Content != "":
 					// Inline content attachment (e.g. pasted text).
 					a.processInlineAttachment(att, &textBuilder)
+				case len(att.Data) > 0:


Routing binary payloads back through ProcessAttachmentWithMetadata is the right call and keeps the size and decompression-bomb guards. Worth a test for the resend path: an already-processed image re-enters ResizeImage, which returns Resized:false when it already fits within limits, so FormatDimensionNote yields no duplicate dimension note. A test would document that idempotence.

Sayt-0 · 2026-07-08T13:36:55Z

 			if note := chat.FormatDimensionNote(resizeMeta); note != "" {
-				textBuilder.WriteString("\n" + note)
+				textBuilder.WriteString("\n")
+				textBuilder.WriteString(note)


Splitting WriteString("\n" + note) into two calls is behavior-preserving but unrelated to attachment extraction. Reverting it keeps the diff scoped to the feature.

Piyush0049 · 2026-07-08T14:56:13Z

I read the review and have worked on it. I have addressed all the requested changes.

Specifically:

Tests: Added a comprehensive regression test in pkg/tui/page/chat/chat_test.go using the newTestChatPage harness that covers the four requested cases (InlineText, InlineData, Legacy format, and Empty document).
Idempotence: Added TestProcessAttachmentWithMetadata_IdempotentResend in pkg/chat/attach_test.go to explicitly verify that an already-resized image yields Resized: false and no duplicate dimension note.
Documentation: Refreshed the stale doc comment on Attachment in session.go to remove the future-tense wording and explicitly describe the inline binary case.
Cosmetic Reversion: Reverted the WriteString split in app.go to keep the diff scoped strictly to the feature.

All tests are passing on my end.

Piyush0049 requested a review from a team as a code owner July 6, 2026 09:15

aheritier added area/tui For features/issues/fixes related to the TUI kind/refactor PR refactors code without behavior change labels Jul 6, 2026

refactor(tui): extract attachments from document parts

85616ba

This commit updates extractAttachmentsFromSession to properly read modern MessagePartTypeDocument attachments instead of relying solely on the legacy 'Contents of' string matching fallback, addressing the TODO in chat.go.

Piyush0049 force-pushed the refactor/extract-attachments branch from 1104c82 to 85616ba Compare July 6, 2026 10:10

dgageot requested changes Jul 6, 2026

View reviewed changes

Sayt-0 reviewed Jul 8, 2026

View reviewed changes

fix: address PR review feedback for attachment extraction

1e98435

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor(tui): extract attachments from document parts in session#3484

refactor(tui): extract attachments from document parts in session#3484
Piyush0049 wants to merge 3 commits into
docker:mainfrom
Piyush0049:refactor/extract-attachments

Piyush0049 commented Jul 6, 2026

Uh oh!

Piyush0049 commented Jul 6, 2026

Uh oh!

dgageot commented Jul 6, 2026

Uh oh!

dgageot left a comment

Uh oh!

Piyush0049 commented Jul 6, 2026

Uh oh!

Piyush0049 commented Jul 6, 2026

Uh oh!

Sayt-0 left a comment

Uh oh!

Sayt-0 Jul 8, 2026

Uh oh!

Sayt-0 Jul 8, 2026

Uh oh!

Sayt-0 Jul 8, 2026

Uh oh!

Sayt-0 Jul 8, 2026

Uh oh!

Piyush0049 commented Jul 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

Piyush0049 commented Jul 6, 2026

Description

Testing

Uh oh!

Piyush0049 commented Jul 6, 2026

Uh oh!

dgageot commented Jul 6, 2026

Uh oh!

dgageot left a comment

Choose a reason for hiding this comment

Uh oh!

Piyush0049 commented Jul 6, 2026

Uh oh!

Piyush0049 commented Jul 6, 2026

Uh oh!

Sayt-0 left a comment

Choose a reason for hiding this comment

Review summary

Uh oh!

Sayt-0 Jul 8, 2026

Choose a reason for hiding this comment

Uh oh!

Sayt-0 Jul 8, 2026

Choose a reason for hiding this comment

Uh oh!

Sayt-0 Jul 8, 2026

Choose a reason for hiding this comment

Uh oh!

Sayt-0 Jul 8, 2026

Choose a reason for hiding this comment

Uh oh!

Piyush0049 commented Jul 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants